Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Metrics for async rpcs #3016

Merged

Conversation

wangdayong228
Copy link
Contributor

@wangdayong228 wangdayong228 commented Dec 27, 2024

This change is Reviewable

@Pana Pana self-requested a review December 30, 2024 10:25
Copy link
Member

@Pana Pana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 9 of 13 files at r1, all commit messages.
Reviewable status: 9 of 13 files reviewed, 3 unresolved discussions (waiting on @wangdayong228)


crates/rpc/rpc-middlewares/src/metrics.rs line 4 at r1 (raw file):

use futures::{future::lazy, FutureExt};
use futures_util::future::BoxFuture;
use jsonrpc_core::Result as RpcResult;

This is a middleware for jsonrpsee framework, recommend directly use Result from jsonrpsee crate


crates/rpc/rpc-builder/Cargo.toml line 45 at r1 (raw file):

tokio = { workspace = true, features = ["full"] }

[dependencies.futures-util]

we should use one crate import style


crates/rpc/rpc-middlewares/Cargo.toml line 18 at r1 (raw file):

jsonrpc-core = { workspace = true }
jsonrpsee = { workspace = true ,features = ["server","ws-client"]}
rustls = { version = "0.21", features = ["dangerous_configuration"] }

use the workspace style

Copy link
Member

@Pana Pana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 9 of 13 files reviewed, 4 unresolved discussions (waiting on @wangdayong228)


Cargo.lock line 1516 at r1 (raw file):

 "futures 0.3.30",
 "futures-util",
 "jsonrpc-core 15.1.0",

this crate should upgraded to 18.0.0 already

Copy link
Contributor Author

@wangdayong228 wangdayong228 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 3 of 13 files reviewed, 4 unresolved discussions (waiting on @Pana)


Cargo.lock line 1516 at r1 (raw file):

Previously, Pana (Pana) wrote…

this crate should upgraded to 18.0.0 already

Done.


crates/rpc/rpc-builder/Cargo.toml line 45 at r1 (raw file):

Previously, Pana (Pana) wrote…

we should use one crate import style

Done.


crates/rpc/rpc-middlewares/Cargo.toml line 18 at r1 (raw file):

Previously, Pana (Pana) wrote…

use the workspace style

Done.


crates/rpc/rpc-middlewares/src/metrics.rs line 4 at r1 (raw file):

Previously, Pana (Pana) wrote…

This is a middleware for jsonrpsee framework, recommend directly use Result from jsonrpsee crate

Done.

Copy link
Member

@Pana Pana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 13 files at r1, 9 of 9 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @wangdayong228)


crates/rpc/rpc-middlewares/src/metrics.rs line 4 at r1 (raw file):

Previously, wangdayong228 (dyoung) wrote…

Done.

delete the commented code


Cargo.lock line 1521 at r2 (raw file):

 "futures 0.3.30",
 "futures-util",
 "jsonrpc-core 18.0.0",

seems jsonrpc-core can be removed from cfx-rpc-middlewares dependencies


crates/rpc/rpc-middlewares/src/throttle.rs line 39 at r2 (raw file):

        let bucket = match self.manager.get(name) {
            Some(bucket) => bucket,
            None => return Ok(()),

If can not get a bucket, seems should return an error ?


crates/rpc/rpc-builder/src/lib.rs line 539 at r2 (raw file):

                ws_server.start(modules.ws.clone().expect("ws server error"))
            });
            return Ok(RpcServerHandle {

maybe both http and ws config are settled, so should not return directly

Copy link
Contributor Author

@wangdayong228 wangdayong228 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 11 of 13 files reviewed, 4 unresolved discussions (waiting on @Pana)


Cargo.lock line 1521 at r2 (raw file):

Previously, Pana (Pana) wrote…

seems jsonrpc-core can be removed from cfx-rpc-middlewares dependencies

Done.


crates/rpc/rpc-builder/src/lib.rs line 539 at r2 (raw file):

Previously, Pana (Pana) wrote…

maybe both http and ws config are settled, so should not return directly

Done.


crates/rpc/rpc-middlewares/src/metrics.rs line 4 at r1 (raw file):

Previously, Pana (Pana) wrote…

delete the commented code

Done.


crates/rpc/rpc-middlewares/src/throttle.rs line 39 at r2 (raw file):

Previously, Pana (Pana) wrote…

If can not get a bucket, seems should return an error ?

Done.

Copy link
Member

@Pana Pana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @wangdayong228)

Copy link
Member

@Pana Pana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @wangdayong228)

@Pana
Copy link
Member

Pana commented Dec 31, 2024

retest this please

1 similar comment
@Pana
Copy link
Member

Pana commented Dec 31, 2024

retest this please

@wangdayong228 wangdayong228 merged commit b8e2238 into Conflux-Chain:master Dec 31, 2024
2 checks passed
@wangdayong228 wangdayong228 deleted the metrics-for-async-rpcs branch December 31, 2024 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants